Skip to content

Allow deleting suspended objects #937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 20, 2022
Merged

Allow deleting suspended objects #937

merged 2 commits into from
Oct 20, 2022

Conversation

darkowlzz
Copy link
Contributor

  • Reorders the object suspended check in all the reconcilers to allow
    deletion of objects when they are suspended. Objects used to get stuck
    on delete because the finalizers were not getting removed due to the
    suspended state.

  • Add setters and getters for spec.suspend and status.artifact in internal/object package.
    This is needed for writing generic tests for any source kind.

  • Adds a generic test for all the reconcilers to check if a suspended
    source object can be delete.

Some observations related to the metrics:

When a suspended object is created, finalizer is set and status.observedGeneration remains in -1.

apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: GitRepository
metadata:
  ...
  finalizers:
  - finalizers.fluxcd.io
  generation: 1
  name: podinfo3
  namespace: default
  ...
status:
  observedGeneration: -1

With prometheus query gotk_reconcile_condition{kind=~"GitRepository",name="podinfo3"}==1, following are the observed metrics:

  • status=Unknown when suspended on creation

gotk_reconcile_condition{container="manager", endpoint="http-prom", exported_namespace="default", instance="10.244.0.21:8080", job="monitoring/flux-system", kind="GitRepository", name="podinfo3", namespace="flux-system", pod="source-controller-8bb478cb9-knmg4", status="Unknown", type="Ready"}

  • status=Ready on resuming

gotk_reconcile_condition{container="manager", endpoint="http-prom", exported_namespace="default", instance="10.244.0.21:8080", job="monitoring/flux-system", kind="GitRepository", name="podinfo3", namespace="flux-system", pod="source-controller-8bb478cb9-knmg4", status="True", type="Ready"}

  • status=Ready when the object is suspended again, same as above.

  • status=Deleted when the suspended object is deleted

gotk_reconcile_condition{container="manager", endpoint="http-prom", exported_namespace="default", instance="10.244.0.21:8080", job="monitoring/flux-system", kind="GitRepository", name="podinfo3", namespace="flux-system", pod="source-controller-8bb478cb9-knmg4", status="Deleted", type="Ready"}

Fixes #934

@darkowlzz darkowlzz added the bug Something isn't working label Oct 18, 2022
stefanprodan
stefanprodan approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @darkowlzz

@stefanprodan
Copy link
Member

Looks like main brach is broken as well:

--- FAIL: TestGitRepositoryReconciler_reconcileSource_authStrategy (29.15s)
    --- FAIL: TestGitRepositoryReconciler_reconcileSource_authStrategy/SSH_with_password_protected_private_key_secret_makes_ArtifactOutdated=True (25.77s)
        --- FAIL: TestGitRepositoryReconciler_reconcileSource_authStrategy/SSH_with_password_protected_private_key_secret_makes_ArtifactOutdated=True/libgit2 (19.21s)
            gitrepository_controller_test.go:543: 
                expected
                	[]v1.Condition{v1.Condition{Type:"FetchFailed", Status:"True", ObservedGeneration:0, LastTransitionTime:time.Date(2022, time.October, 11, 15, 7, 2, 0, time.UTC), Reason:"GitOperationFailed", Message:"failed to checkout and determine revision: recovered from git2go panic: runtime error: invalid memory address or nil pointer dereference"}}
                to match
                	[]v1.Condition{v1.Condition{Type:"ArtifactOutdated", Status:"True", ObservedGeneration:0, LastTransitionTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Reason:"NewRevision", Message:"new upstream revision 'master/9a6150b9f28091197cb5c0e6f52a50f2fa07b6bd'"}, v1.Condition{Type:"Reconciling", Status:"True", ObservedGeneration:0, LastTransitionTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Reason:"NewRevision", Message:"new upstream revision 'master/9a6150b9f28091197cb5c0e6f52a50f2fa07b6bd'"}}

@darkowlzz
Copy link
Contributor Author

Looks like main brach is broken as well

failed to checkout and determine revision: recovered from git2go panic: runtime error: invalid memory address or nil pointer dereference

Seems to be because of git2go panic, which is resulting in a different status condition. Also, only panicking on macOS 11.

@stefanprodan
Copy link
Member

Ok we can ignore the panic then, it's not something we can fix in this PR.

@darkowlzz
internal/object: setter/getter suspend & artifact
15b4f96 
Add setters and getters for spec.suspend and status.artifact.
This is needed for writing generic tests for any source kind.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz
controllers: Allow deletion of suspended objects
15cdd85 
Reorders the object suspended check in all the reconcilers to allow
deletion of objects when they are suspended. Objects used to get stuck
on delete because the finalizers were not getting removed due to the
suspended state.

Adds a generic test for all the reconcilers to check if a suspended
source object can be delete.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@stefanprodan stefanprodan mentioned this pull request Oct 20, 2022
Closed
15 tasks
@stefanprodan stefanprodan merged commit f9e6d05 into main Oct 20, 2022
@stefanprodan stefanprodan deleted the delete-suspended branch October 20, 2022 14:39
@pjbgf pjbgf added this to the GA milestone Oct 20, 2022
@darkowlzz darkowlzz mentioned this pull request Dec 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stefanprodan stefanprodan stefanprodan approved these changes

@hiddeco hiddeco hiddeco approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

Maintainers' Focus
Status: Done

Milestone

Helm GA

Development

Successfully merging this pull request may close these issues.

Suspended sources can't be deleted

4 participants

@darkowlzz @stefanprodan @hiddeco @pjbgf